-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove dependency on rmw_implementation #102
remove dependency on rmw_implementation #102
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Seems like the only missing piece is to delete the manually generated typesupport files in rmw_fastrtps_cpp
and rmw_fastrpts_dynamic_cpp
.
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
8d82073
to
e46ff4e
Compare
… variable passed as an argument Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
set(_typesupport_impls "") | ||
call_for_each_rmw_implementation(accumulate_typesupports) | ||
foreach(_typesupport_impl ${_typesupport_impls}) | ||
list_append_unique(TYPESUPPORT_IMPLS ${_typesupport_impl}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous logic was broken. The variable which name is passed in TYPESUPPORT_IMPLS
is never set by the macro. This only happened to work if the caller expected the result in _typesupport_impls
. 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cmake
macros are really tricky.
@@ -36,8 +35,6 @@ | |||
<test_depend>python3-pytest</test_depend> | |||
<test_depend>python_cmake_module</test_depend> | |||
<test_depend>rmw</test_depend> | |||
<test_depend>rmw_implementation</test_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this change led to a failure on the binary job. We're generating messages for tests, but there's no C typesupport available (previously transitively brought in by rmw_implementation
).
Here is a proposed fix: #110
Needed for ros2/rmw_dds_common#12